-
Notifications
You must be signed in to change notification settings - Fork 12.2k
ERC4626: Allow overriding underlying assets transfer mechanisms #5970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: ea74661 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Walkthrough
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contracts/token/ERC20/extensions/ERC4626.sol (1)
302-310
: Well-designed hooks with clear default implementations.The introduction of
_transferIn
and_transferOut
as internal virtual functions elegantly solves the issue raised in #5960. Key strengths:
- The
virtual
modifier enables customization without forcing implementers to skipsuper._deposit
/super._withdraw
- Default implementations maintain backward compatibility using
SafeERC20
- Clear function names that indicate directionality
- Proper separation of concerns
Optional: Consider enhancing documentation.
While the current brief comments are adequate, you could optionally add more detailed NatSpec documentation for these hooks, such as:
- /// @dev Performs a transfer in of underlying assets. The default implementation uses `SafeERC20`. Used by {_deposit}. + /** + * @dev Performs a transfer in of underlying assets from `from` to the vault. + * + * The default implementation uses `SafeERC20.safeTransferFrom`. Used by {_deposit}. + * + * IMPORTANT: When overriding this function: + * - Ensure assets are securely transferred or accounted for + * - Consider updating {totalAssets} if assets are held elsewhere (e.g., staking) + * - Maintain reentrancy safety by preserving transfer-before-mint order in {_deposit} + * - Update preview functions if the override affects conversion rates + */Additionally, the contract header's override guidance (lines 52-68) could mention these hooks as another customization point alongside
_deposit
and_withdraw
.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/spotty-plums-brush.md
(1 hunks)contracts/token/ERC20/extensions/ERC4626.sol
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: slither
- GitHub Check: tests-upgradeable
- GitHub Check: coverage
- GitHub Check: tests-foundry
- GitHub Check: tests
- GitHub Check: Redirect rules - solidity-contracts
- GitHub Check: halmos
🔇 Additional comments (3)
contracts/token/ERC20/extensions/ERC4626.sol (2)
270-270
: LGTM! Clean refactor enabling customization.The replacement of the direct
SafeERC20.safeTransferFrom
call with_transferIn
successfully achieves the PR objective of making asset transfers overridable. The order of operations (transfer before mint) is preserved, maintaining the reentrancy safety properties documented in the comment above.
297-297
: LGTM! Clean refactor enabling customization.The replacement of the direct
SafeERC20.safeTransfer
call with_transferOut
successfully achieves the PR objective of making asset transfers overridable. The order of operations (burn before transfer) is preserved, maintaining the reentrancy safety properties documented in the comment above..changeset/spotty-plums-brush.md (1)
1-5
: LGTM! Accurate changeset documentation.The changeset correctly:
- Classifies this as a
minor
version bump, which is appropriate for adding new internal virtual functions while maintaining backward compatibility- Provides a clear, concise description of the enhancement
- Accurately identifies the two new functions (
_transferIn
and_transferOut
)
Fixes #5960
PR Checklist
npx changeset add
)